Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick · 2024-04-08T22:19:44Z

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this:

An internal implementation of ArrayBufferWriter is added for platforms that do not support it
SqlObjectPool is imbued with the ability to create/reset pooled objects
TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs
Native methods are updated to take in Span/* for writeable byte[]
SSPIContextProvider signature is updated to take IBufferWriter

Part of #2253

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this: - An internal implementation of ArrayBufferWriter is added for platforms that do not support it - SqlObjectPool is imbued with the ability to create/reset pooled objects - TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs - Native methods are updated to take in Span/* for writeable byte[] - SSPIContextProvider signature is updated to take IBufferWriter

twsouthwick · 2024-05-07T21:20:34Z

@David-Engel can I get a review on this next step in the SSPI journey?

codecov · 2024-05-08T03:24:57Z

Codecov Report

Attention: Patch coverage is 78.35821% with 29 lines in your changes missing coverage. Please review.

Project coverage is 66.19%. Comparing base (0be2756) to head (e17288c).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
.../src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs	56.25%	21 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	85.41%	7 Missing ⚠️
...t/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs	83.33%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (e17288c). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (0be2756) HEAD (e17288c)

addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2452      +/-   ##
==========================================
- Coverage   72.81%   66.19%   -6.63%     
==========================================
  Files         282      277       -5     
  Lines       59112    58829     -283     
==========================================
- Hits        43045    38944    -4101     
- Misses      16067    19885    +3818

Flag	Coverage Δ
addons	`?`
netcore	`69.18% <89.15%> (-6.34%)`	⬇️
netfx	`64.87% <77.51%> (-6.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twsouthwick · 2024-05-13T18:04:31Z

@David-Engel ping

David-Engel · 2024-05-13T23:39:20Z

I ran our internal Kerberos test pipeline against this PR and all tests passed.

twsouthwick · 2024-05-28T20:16:11Z

ping @JRahnama @DavoudEshtehari @arellegue

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs

twsouthwick · 2024-08-19T17:48:42Z

@David-Engel can you run this on the kerberos suite again?

twsouthwick · 2024-08-20T16:50:52Z

@David-Engel something is up with the merges so let me get those fixed before you do the kerberos runs

twsouthwick · 2024-08-20T18:24:10Z

@David-Engel I'm getting sporadic errors that don't seem to always repro: https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=96359&view=logs&j=b14281d2-3365-502e-c6c8-e9e46d660715&t=a67f9feb-8bad-50ac-92c7-d62292e3e6fb&l=63

Have you seen these or are they related to my pr?

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

DavoudEshtehari · 2024-08-21T02:12:09Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


No complain here only I'm curious about the constant 20 pooled objects here!
cc @David-Engel

honestly I don't remember where the 20 came from. Seemed reasonable I think at the time, but I'm not really aware of how many concurrent calls to it are called in a given connection

I dug into the SqlObjectPool logic because I was curious, so thought I would share in case anyone else was wondering. 20 here isn't a limit on parallelism or anything. It's just the maximum number that will be cached. Parallel Rents from the pool above 20 will simply get a new object. I agree that 20 should be plenty for the majority of scenarios.

Can you please add a comment in the code indicating the reasoning for 20?

azure-pipelines · 2025-01-31T22:50:35Z

Azure Pipelines successfully started running 1 pipeline(s).

twsouthwick · 2025-02-01T01:39:12Z

@David-Engel @cheenamalhotra This PR is passing all tests and ready for review. Thanks!

twsouthwick · 2025-02-01T01:40:55Z

...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs

@@ -842,145 +842,6 @@ internal void WriteByte(byte b)
            _outBuff[_outBytesUsed++] = b;
        }

-        internal Task WriteByteSpan(ReadOnlySpan<byte> span, bool canAccumulate = true, TaskCompletionSource<object> completion = null)


Note: this is removed in the netcore/netfx specific files, and the netcore implementation is now in the shared TdsParserStateObject class

twsouthwick · 2025-02-13T16:25:44Z

@David-Engel these build failures appear to be occurring on main as well. Can I get a review here?

David-Engel · 2025-02-13T17:26:45Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


I dug into the SqlObjectPool logic because I was curious, so thought I would share in case anyone else was wondering. 20 here isn't a limit on parallelism or anything. It's just the maximum number that will be cached. Parallel Rents from the pool above 20 will simply get a new object. I agree that 20 should be plenty for the majority of scenarios.

twsouthwick · 2025-02-13T17:50:14Z

Looks like the "restore" step is failing for a bunch of the tests

David-Engel · 2025-02-13T17:58:30Z

Looks like the "restore" step is failing for a bunch of the tests

Yup. It's not your PR. It seems like a new underlying tooling bug on ARM64. We are investigating.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

mdaigle · 2025-02-13T22:00:18Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


Can you please add a comment in the code indicating the reasoning for 20?

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

twsouthwick · 2025-02-24T18:17:38Z

@mdaigle I've handled the merge conflicts

mdaigle · 2025-02-24T20:00:46Z

/azp run

azure-pipelines · 2025-02-24T20:01:08Z

Azure Pipelines successfully started running 1 pipeline(s).

twsouthwick · 2025-02-26T17:07:55Z

@mdaigle @David-Engel Is this good to merge in? Looks like main is still not running clean so I don't think my PR introduced the test failure I'm seeing

mdaigle · 2025-02-26T17:21:18Z

I kicked the failing job and expect it to pass shortly, then I'll merge 😄

This was referenced Apr 8, 2024

Add hook for custom SSPI context for SqlConnection instances #2253

Open

Expose SqlAuthenticationParameters on SSPIContextProvider #2454

Draft

twsouthwick force-pushed the sspi-writer branch from e52f1c3 to 1ec663f Compare May 7, 2024 16:28

twsouthwick marked this pull request as ready for review May 7, 2024 21:20

David-Engel approved these changes May 13, 2024

View reviewed changes

David-Engel requested review from JRahnama, DavoudEshtehari and arellegue May 13, 2024 23:38

David-Engel added this to the 6.0-preview1 milestone Jun 3, 2024

twsouthwick added 4 commits June 27, 2024 11:15

Merge branch 'main' into sspi-writer

7c3335e

Merge remote-tracking branch 'upstream/main' into sspi-writer

4c3c3f2

fix

d3bd2b9

switch to span

266cf7f

David-Engel reviewed Jul 31, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs Outdated Show resolved Hide resolved

add return

7d99053

twsouthwick mentioned this pull request Aug 19, 2024

Use string for SNI instead of byte[] #2790

Open

twsouthwick added 3 commits August 19, 2024 11:17

Merge remote-tracking branch 'upstream/main' into sspi-writer

7e4d15f

revert

09db047

use return

a49abc3

inline

a1703d8

DavoudEshtehari reviewed Aug 21, 2024

View reviewed changes

twsouthwick commented Feb 1, 2025

View reviewed changes

twsouthwick force-pushed the sspi-writer branch from 74f7138 to c43f86f Compare February 3, 2025 18:59

make sure to use correct length

7b7ee36

twsouthwick force-pushed the sspi-writer branch from c43f86f to 7b7ee36 Compare February 3, 2025 21:38

Merge branch 'main' into sspi-writer

4f95969

paulmedynski requested review from a team and removed request for JRahnama and arellegue February 11, 2025 19:03

twsouthwick added 2 commits February 12, 2025 09:56

Merge remote-tracking branch 'origin/main' into sspi-writer

e1986ac

put method back

130cdef

David-Engel approved these changes Feb 13, 2025

View reviewed changes

mdaigle requested a review from a team February 13, 2025 17:32

mdaigle reviewed Feb 13, 2025

View reviewed changes

twsouthwick added 2 commits February 13, 2025 14:46

Add comment for pool

f40013e

Add note about file origin and editing restrictions

31e3ec9

mdaigle approved these changes Feb 14, 2025

View reviewed changes

Merge branch 'main' into sspi-writer

e17288c

mdaigle added this to the 7.0-preview1 milestone Feb 26, 2025

mdaigle merged commit dcf6ac4 into dotnet:main Feb 26, 2025
123 checks passed

twsouthwick deleted the sspi-writer branch February 26, 2025 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick commented Apr 8, 2024 •

edited

Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 •

edited

Loading

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

DavoudEshtehari Aug 21, 2024

twsouthwick Nov 15, 2024

David-Engel Feb 13, 2025

mdaigle Feb 13, 2025

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Feb 1, 2025

twsouthwick Feb 1, 2025

twsouthwick commented Feb 13, 2025

David-Engel Feb 13, 2025

twsouthwick commented Feb 13, 2025

David-Engel commented Feb 13, 2025

mdaigle Feb 13, 2025

twsouthwick commented Feb 24, 2025

mdaigle commented Feb 24, 2025

azure-pipelines bot commented Feb 24, 2025

twsouthwick commented Feb 26, 2025

mdaigle commented Feb 26, 2025

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Conversation

twsouthwick commented Apr 8, 2024 • edited Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 • edited Loading

Codecov Report

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

DavoudEshtehari Aug 21, 2024

Choose a reason for hiding this comment

twsouthwick Nov 15, 2024

Choose a reason for hiding this comment

David-Engel Feb 13, 2025

Choose a reason for hiding this comment

mdaigle Feb 13, 2025

Choose a reason for hiding this comment

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Feb 1, 2025

twsouthwick Feb 1, 2025

Choose a reason for hiding this comment

twsouthwick commented Feb 13, 2025

David-Engel Feb 13, 2025

Choose a reason for hiding this comment

twsouthwick commented Feb 13, 2025

David-Engel commented Feb 13, 2025

mdaigle Feb 13, 2025

Choose a reason for hiding this comment

twsouthwick commented Feb 24, 2025

mdaigle commented Feb 24, 2025

azure-pipelines bot commented Feb 24, 2025

twsouthwick commented Feb 26, 2025

mdaigle commented Feb 26, 2025

twsouthwick commented Apr 8, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading